[v2] Refactor ReanimatedModal/index.tsx to follow Rules of React and compile with React Compiler#90555
[v2] Refactor ReanimatedModal/index.tsx to follow Rules of React and compile with React Compiler#90555roryabraham wants to merge 18 commits into
Conversation
…Compiler - Group useEffects by responsibility; co-locate cleanup with creation - Extract transition callbacks with useEffectEvent to fix stale closure - Derive isTransitioning and eliminate redundant isVisibleState state - Remove manual memoization (useCallback/useMemo) superseded by React Compiler - DRY up clearTransitionHandles helper - Sort component internals: hooks, refs, derived, callbacks, effects Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Covers four staging regressions from the React Compiler refactor: - #90438 RHP no animation (derived isTransitioning loses "in-progress" semantics on oscillation) - #90442 Android can't confirm deletion (interaction handle cleared prematurely) - #90463 Android modal not displayed (interaction handle cleared prematurely) - #90510 Web flickering on close (Container unmounted before exit animation plays) Tests are intentionally red until the fixes are applied. Co-authored-by: Cursor <cursoragent@cursor.com>
Wraps onBackButtonPressHandler and handleEscape in useEffectEvent so they hold a stable reference across renders without needing deps. The back handler effect can now use [] as its dep array, eliminating re-registration on every render cycle. Fixes #90442 and #90463. Co-authored-by: Cursor <cursoragent@cursor.com>
Replaces two booleans (isContainerOpen + derived isTransitioning) with a single explicit ModalState enum: 'closed' | 'opening' | 'open' | 'closing'. The enum makes impossible states impossible and fixes oscillation bugs: - modalState only advances via animation callbacks (onOpenCallBack / onCloseCallBack), so it can never flip back due to isVisible oscillating. - The isVisible → state transition uses the render-time state adjustment pattern instead of a useEffect, eliminating extra renders. - Container visibility uses (modalState === 'opening' || modalState === 'open') so the Container unmounts when closing begins, triggering Reanimated's ghost- node exit animation, and does not re-mount during isVisible oscillation. Updates regression tests to use reliable spy-count assertions; removes dependency on InteractionManager.runAfterInteractions which is mocked to fire immediately in the test environment. Fixes #90438, #90510. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract TransitionTracker and InteractionManager handle management out of React state and into a new useAnimationTransition hook. Handles now start on component mount/unmount (via useLayoutEffect) and end when the animation callback fires (onAnimationComplete), bypassing React state as an intermediary. This fixes a class of bugs where React's re-render cycle could clear or create handles at the wrong time, and removes the useEffect that was responsible for the premature cleanup regressions. Update the Container mock in tests to use the real useAnimationTransition hook so spy assertions on TransitionTracker and InteractionManager remain meaningful. Remove the useEffect safety net from useAnimationTransition as it ran synchronously on unmount, immediately ending the exit-animation handle before Reanimated's ghost-node could complete its animation. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Add useOnValueChange<T>(value, onChange) — a thin wrapper around React's render-time state adjustment pattern. Runs onChange synchronously during render (no intermediate DOM commit), unlike useEffect which fires after paint. Uses Object.is for comparison to match React's own semantics. Use it in ReanimatedModal to replace the manual prevIsVisible state variable. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
GestureHandler and Container already default swipeThreshold to 100, so the required constraint on the prop type was unnecessarily strict. Co-authored-by: Cursor <cursoragent@cursor.com>
Modals don't require content to render — the prop should be optional to allow unit tests and other callers to omit it. Co-authored-by: Cursor <cursoragent@cursor.com>
When a modal is mounted with isVisible=true (e.g. ConfirmModalWrapper), useOnValueChange only triggers on changes, so the initial 'closed' state was never advanced, preventing the modal content from ever rendering. Co-authored-by: Cursor <cursoragent@cursor.com>
16a034d to
d62eb4d
Compare
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
This comment was marked as outdated.
This comment was marked as outdated.
…er with React Compiler - Backdrop/index.tsx: inline BackdropOverlay JSX variable (partial render anti-pattern) - Container/index.tsx: remove manual useMemo for Entering/Exiting; replace Partial<ReanimatedModalProps> with self-contained ContainerProps - Container/index.web.tsx: remove manual useMemo for Exiting; add animationIn to useAnimatedStyle deps - GestureHandler.tsx: remove manual useMemo for panGesture - types.ts: expand ContainerProps with native-only fields; remove animationInDelay from BackdropProps (never consumed) - index.tsx: remove animationInDelay pass-through to Container and Backdrop Co-authored-by: Cursor <cursoragent@cursor.com>
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Co-authored-by: Cursor <cursoragent@cursor.com>
|
requested AdHoc testing: https://expensify.slack.com/archives/C09V78U42D8/p1778795508021849 |
FAB - Unable to open global FABVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:User is able to open FAB options Actual Result:FAB options are not opened on Web. Native apps becomes unresponsive/ crashes Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
Unable to dismiss image attachementVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:Image should be closed Actual Result:Image can not be closed Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
Expense report - More modal blinks after being dismissedVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:The modal is closed Actual Result:The modal blinks after being dismissed Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
Delete modal shake and do not responseVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:Message should be deleted Actual Result:Modal start shaking and can not be dismissed for a while Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
Workspace - App crashes when trying to delete a category/tagVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:The user is able to delete the category/tag Actual Result:App crashes Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
Chat - App crashes when long tapping on a messageVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:The context menu is opened Actual Result:The app crashes Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
Troubleshoot - App crashes tapping Clear cache and restartVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:App will not crash tapping Clear cache and restart Actual Result:App crashes tapping Clear cache and restart Workaround:Unknown Platforms:
Screenshots/VideosBug7157018_1779101998995!iPhone_Ilona_Console.txt Bug7157018_1779101998995.ScreenRecording_05-18-2026_05-52-13_1.mp4 |
App crashes on tapping explore all plans.Version Number: V9.3.74-0 PR:90555 Action Performed:
Expected Result:App must not crash on tapping explore all plans. Actual Result:App crashes on tapping explore all plans. Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
App crashes on tapping plus icon in workspace chat.Version Number: V9.3.74-0 PR:90555 Action Performed:
Expected Result:App must not crash on tapping plus icon in workspace chat. Actual Result:App crashes on tapping plus icon in workspace chat. Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
App crashes on tapping status emoji dropdownVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:App must not crash on tapping status emoji dropdown Actual Result:App crashes on tapping status emoji dropdown Workaround:Unknown Platforms:
Screenshots/VideosBug7157028_1779102759499.Screenrecorder-2026-05-18-16-41-15-234.mp4 |
App crashes on tapping 3 dots next to WorkspaceVersion Number: V9.3.74 Action Performed:
Expected Result:App must not crash on tapping 3 dots next to Workspace Actual Result:App crashes on tapping 3 dots next to Workspace Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
iOS - Avatar - App crashes uploading an avatarVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:App will not crash Actual Result:App crashes uploading an avatar Workaround:Unknown Platforms:
Screenshots/VideosBug7157043_1779104008720!crash_.txt Bug7157043_1779104008728.ScreenRecording_05-18-2026_06-30-57_1.mp4 |
App crashes on tapping import onyx stateVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:App must not crash on tapping import onyx state Actual Result:App crashes on tapping import onyx state Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
Copilot - App crashes taping the 2 arrow icon next to the account sectionVersion Number: v9.3.74-0 PR:90555 Action Performed:Precondition: User has access to any account as a copilot.
Expected Result:App will not crash tapping the 2 arrow icon next to the account section Actual Result:App crashes taping the 2 arrow icon next to the account section Workaround:Unknown Platforms:
Screenshots/VideosBug7157088_1779107208170!crash.txt Bug7157088_1779107208189.ScreenRecording_05-18-2026_07-25-48_1.mp4 |
App crashes on tapping cog / drop-down filter chipVersion Number: 9.3.74 Action Performed:
Expected Result:App must not crash on tapping cog / drop-down filter chip. Actual Result:App crashes on tapping cog / drop-down filter chip. Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
App crashes on tapping more in members invite pageVersion Number: v9.3.74-0 PR:90555 Action Performed:Precondition: User has access to any account as a copilot.
Expected Result:App will not crash tapping the 2 arrow icon next to the account section Actual Result:App crashes taping the 2 arrow icon next to the account section Workaround:Unknown Platforms:
Screenshots/VideosBug7157102_1779111064501!Crash.txt Bug7157102_1779108728874.Bug7157035_1779103529958_Screenrecorder-2026-05-18-16-54-01-654.mp4 |
Distance rates - App crashes when opening distance rate unitVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:The change unit page is opened Actual Result:The app crashes Workaround:Unknown Platforms:
Screenshots/VideosBug7157121_1779109883815.video_2026-05-18_16-09-49.mp4 |
Card - App crashes on tapping update currencyVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:App must not crash on tapping update currency Actual Result:App crashes on tapping update currency Workaround:Unknown Platforms:
Screenshots/VideosBug7157151_1779111821605.Screenrecorder-2026-05-18-19-09-24-126.mp4 |
In bank flow, tapping date picker app crashesVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:In bank flow, tapping date picker app must not crash Actual Result:In bank flow, tapping date picker app crashes Workaround:Unknown Platforms:
Screenshots/VideosBug7157153_1779112298488.Screenrecorder-2026-05-18-19-18-20-199.mp4 |
App crashes on opening tax name fieldVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:App must not crash on opening tax name field Actual Result:App crashes on opening tax name field Workaround:Unknown Platforms:
Screenshots/VideosBug7157212_1779116573930.Screenrecorder-2026-05-18-20-30-10-863.mp4 |
App crashes after tapping "Submit" on report and can´t be relaunched anymoreVersion Number: v9.3.74-0 PR:90555 Action Performed:Prerequisite: Account has no expenses yet.
Expected Result:User should be able to continue using the app normally after submitting an expense. Actual Result:After tapping on "Submit" inside an expense report, app crashes and can´t be relaunched anymore, as it keeps crashing. The only way to be able to use the app again is by reinstalling it. Workaround:Unknown Platforms:
Screenshots/VideosBug7157227_1779117783396.Submit_Crash.mp4 |
App crashes on tapping multiple receipt iconVersion Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:App must not crash on tapping multiple receipt icon. Actual Result:App crashes on tapping multiple receipt icon. Workaround:Unknown Platforms:
Screenshots/VideosBug7157238_1779119046666.Screenrecorder-2026-05-18-21-11-07-206.mp4 |
Share - App crashes on try to share file.Version Number: v9.3.74-0 PR:90555 Action Performed:
Expected Result:App must not crash on try to share file. Actual Result:App crashes on try to share file. Workaround:Unknown Platforms:
Screenshots/VideosVideo.mp4 |
Explanation of Change
This is v2 of #90358, which was reverted due to staging regressions. This version contains the original refactoring changes plus fixes for all four reported regressions.
Regressions fixed:
Root cause: The original refactor derived
isTransitioningfromisVisible !== isContainerOpen. WhenisVisibleoscillated back to matchisContainerOpenmid-animation (e.g. rapid RHP navigation),isTransitioningincorrectly becamefalse, prematurely ending transition handles and allowing the Container to re-mount mid-animation.Fix: Replace the two booleans with a four-state enum (
closed | opening | open | closing). State advances only from stable states (closed → opening,open → closing) and is latched through animations, so mid-animation oscillations are ignored. MoveTransitionTracker/InteractionManagerhandle management out of React state and into a newuseAnimationTransitionhook that ties directly to component mount/unmount and animation callbacks, bypassing React's render cycle.Fixed Issues
$
PROPOSAL:
Tests
Regression #90510 — web flash on RHP close:
5. On web, open the Search RHP (tap the search icon in the Inbox header).
6. Close it by clicking outside or pressing Escape.
7. Verify the chat list does not briefly reappear over the area where the RHP was.
Regression #90438 — RHP animation missing after navigation:
8. On web, go to Workspace Settings → Distance rates → Settings → Unit.
9. Select a different unit and navigate back.
10. Verify the RHP animates on every open/close without getting stuck unanimated.
Regression #90463 — modal not displayed on Android:
11. On Android: Workspace → Tags → More → Import spreadsheet → Multi-level tags → Upgrade → Got it, thanks → Switch Tag Levels.
12. Verify the Import spreadsheet modal appears with a "Choose file" button.
Regression #90442 — confirmation modal missing on Android:
13. On Android: create an expense → open the expense report → More → Delete → Delete.
14. Verify the expense is successfully deleted. Also verify delete/remove actions work for: saved searches, workspace members, categories, tags, distance rates, taxes, per diem rates.
Offline tests
N/A — no offline-specific behavior changed.
QA Steps
Regression #90510 — web flash on RHP close (web):
4. Navigate to Inbox → click the search icon → close the Search RHP by clicking outside or pressing Escape.
5. Verify chats do not briefly reappear on screen where the RHP was displayed.
Regression #90438 — RHP animation stops (web):
6. Go to Workspace Settings → Distance rates → Settings → Unit → select another unit.
7. Verify the RHP animates on every subsequent open/close.
Regression #90463 — modal not displayed on Android:
8. On Android: Workspace → Tags → More → Import spreadsheet → Multi-level tags → Upgrade → Got it, thanks → Switch Tag Levels.
9. Verify the Import spreadsheet modal appears with a "Choose file" button.
Regression #90442 — confirmation modal missing on Android:
10. On Android: create an expense → open the expense report → More → Delete → Delete.
11. Verify the expense is successfully deleted. Also verify delete/remove actions work for: saved searches, workspace members, categories, tags, distance rates, taxes, per diem rates.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Made with Cursor
/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos